-
Notifications
You must be signed in to change notification settings - Fork 5.1k
Don't use incompatible LOAD_LIBRARY_SEARCH flags when using LOAD_WITH_ALTERED_SEARCH_PATH #111990
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…_ALTERED_SEARCH_PATH
b3cc511
to
5795fbd
Compare
5795fbd
to
e612989
Compare
Should we reflect this in documentation somewhere? I'm also interested in if this is break change worthy? Was anyone being successful prior to this change? |
Where were you thinking? I see this as implementation detail of how the runtime uses the public/documented OS API
I didn't find anything that would make this a breaking change. Without this change, if the library being loaded didn't need to load another native dependency, it could be loaded - that remains true with this change. If the library isn't in the assembly directory, we try to load with the other specified flags just as before. |
That is a fair point and the question was sort of related to my second. I agree this is an incorrect usage so we fixed that, yay. It is really around is anyone going to be surprised that we are now filtering out these settings? Which really means was anyone previously being successful, which sounds like they weren't. |
The test run on this was several months old. The changes interacted poorly with what was done in the meantime and introduced multiple test failures hit on every CoreCLR PR, e.g. https://dev.azure.com/dnceng-public/public/_build/results?buildId=1014255&view=ms.vss-test-web.build-test-results-tab |
For the #114595 issue specifically, I think this failure was related to #112359. It can be fixed by adding these links to string(REPLACE "/DEPENDENTLOADFLAG:0x800" "" CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS}")
string(REPLACE "/DEPENDENTLOADFLAG:0x800" "" CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS}") |
LoadLibraryEx documents that the
LOAD_WITH_ALTERED_SEARCH_PATH
flag is incompatible withLOAD_LIBRARY_SEARCH
flags. In cases where we explicitly add this flag (absolute path or looking in p/invoke assembly directory), we could end up specifying an incompatible combination if otherDllImportSearchPath
flags corresponding to theLOAD_LIBRARY_SEARCH
flags were also specified. This change avoids using those flags in when the runtime specifies loading with an altered search path.Fixes #111825